Skip to content

Conversation

@sla8c
Copy link
Contributor

@sla8c sla8c commented Dec 14, 2022

This PR adds functionality to fix a caching issue in the PeriodStore

The StatsPeriodStore has a note on how the persist to CoreData has an unintuitive call. And after diving deeply + trying to diagnose I can see that at the tail end of fetchAsyncData no further saves to CoreData are called

Attempted solution 1

I've tried to fix this by adding a barrierblock as the last operation to fetchAsyncData but at this time that is still not working so I've left this in draft status

Note that all the other fetch calls in StatsPeriodStore also save to core data ie: fetchAllVideos, fetchAllSearchTerms etc so I'm not sure why fetchAsyncData did not have this as well and cannot see why it shouldn't have (this call)

Attempted solution 2

The barrierblock solution was discarded in favor of using dispatch groups to save to core data once multiple fetches have completed.

Fixes #19430

These should be the steps for a successful test (same as in the issue)

  1. Enable new stats feature flags
  2. Go to Stats screen and add Views & Visitors card
  3. Tap back and return to main stats screen
  4. The total numbers on Views & Visitors should hold their cached value

Regression Notes

  1. Potential unintended areas of impact
    Stats

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@sla8c sla8c marked this pull request as draft December 14, 2022 17:52
@sla8c sla8c requested review from guarani and staskus December 14, 2022 17:52
@sla8c sla8c added this to the 21.5 milestone Dec 14, 2022
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 14, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19776-f2a8f24 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 14, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19776-f2a8f24 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@staskus
Copy link
Contributor

staskus commented Dec 20, 2022

@sla8c , I looked at this change and I agree with you that it makes perfect sense.

I also tried to understand why it might not be working. I found that PeriodOperation (StatsPeriodAsyncOperation) is not marked as finished, therefore the queue never triggers barrierBlock. I'm not sure why we're not marking the operation as finished after we receive the data.

This is a quick change that I made to StatsPeriodAsyncOperation:

    override func main() {
        service?.getData(for: period, endingOn: date, limit: limit) { [unowned self] (type: TimeStatsType?, error: Error?) in
            if self.isCancelled {
                self.state = .isFinished
                return
            }

            self.completion(type, error)
            self. State = .isFinished // <- code of line that was added
        }
    }

After this change, the block is triggered:

image

@sla8c
Copy link
Contributor Author

sla8c commented Dec 21, 2022

@staskus as discussed I've pushed a change for this using DispatchGroups and its ready for review. Thanks! 🙇

@sla8c sla8c marked this pull request as ready for review December 21, 2022 15:48
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guarani let me know if you'd like to review as well.

I'll wait for your take. We can make any improvements if needed ourselves.

@guarani
Copy link
Contributor

guarani commented Dec 22, 2022

@guarani let me know if you'd like to review as well.

👋 @staskus, sure – I'd like to review this now.

I'll wait for your take. We can make any improvements if needed ourselves.

Ok, sounds like a plan 👍

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix works great! I found I verified that after the app gets new Stats data for Views, it uses that on subsequent loads of the Stats page.

I'm not sure about a couple of things:

  • Is saving to core data only needed when the feature flag is on (and not when it's off)?
  • Is saving to core data even when there's an error fetching the data the right choice?
    I'll come back to this to finish digging into these questions.

@staskus
Copy link
Contributor

staskus commented Dec 23, 2022

  • Is saving to core data only needed when the feature flag is on (and not when it's off)?

If we don't add Feature Flags, we would make this functionality available for the non-revamped version of Stats as well. I tested and I don't see a problem with that, outside the fact that this improvement then could get shipped before the actual revamped stats.

  • Is saving to core data even when there's an error fetching the data the right choice?

No issues happen because of that but there's a bit of inefficiency.

If an error happens for all the calls, then there's no change in data in the state, and the same information gets persisted again.

If an error happens for one or two of the calls and the others succeed then we're correct to persist data.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving this PR since the functionality works for me and the usage of DispatchGroup in this scenario is understandable.

As I explained in the previous comment: usage of FeatureFlag could potentially be optional and I don't see any issues that could come with persisting data if some of the calls fail.

@staskus staskus requested a review from guarani December 28, 2022 08:16
@staskus
Copy link
Contributor

staskus commented Jan 2, 2023

Merging with one approval. Since the functionality is left under the feature flag it's safe to merge.

@staskus staskus merged commit 21e4fdb into trunk Jan 2, 2023
@staskus staskus deleted the issue/stats-period-core-data-persist branch January 2, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[iOS] Stats: Views and Visitors card always show outdated cached totals first

5 participants